Repository navigation
Conversation
|
Claude finished @jcastro-dotcms's task in 3m 1s —— View job Code reviewReviewed against New IssuesNone beyond the already-reported items below. Existing (still present)
Resolved / not a defect
Notes
None of the above block the stacked rollout — all are flag-on only (default off), with the Postgres-only SQL being the most actionable since it diverges from the PR's own portable write. |
|
Pull Request Unsafe to Rollback!!!
|
![semgrep-dotcms[bot]](https://avatars.githubusercontent.com/u/1005263?s=60&v=4){kind=small}
| final var saved = new DotConnect().setSQL("update job set parameters = parameters || ?::jsonb, " | ||
| + "updated_at = current_timestamp where id = ? and queue_name = ? " | ||
| + "and coalesce(parameters->>'afterInode', '') = ? returning id") |
There was a problem hiding this comment.
🟡 Medium severity blocking issue identified in your code:
The method identified is susceptible to injection. The input should be validated and properly
escaped.
Why this might be safe to ignore:
The SQL is assembled only from fixed application-controlled fragments, while all variable values are supplied through parameter binding with placeholders. No attacker-controlled input reaches the SQL syntax, so this is a false positive.
To resolve this comment:
🔧 No guidance has been designated for this issue. Fix according to your organization's approved methods.
💬 Ignore this finding
Reply with Semgrep commands to ignore this finding.
/fp <comment>for false positive/ar <comment>for acceptable risk/other <comment>for all other reasons
Alternatively, triage in Semgrep AppSec Platform to ignore the finding created by CUSTOM_INJECTION-2.
If this is a critical or high severity finding, please also link this issue in the #security channel in Slack.
You can view more details about this finding in the Semgrep AppSec Platform.
| final var rows = new DotConnect().setSQL("select inode, identifier, contentlet_as_json " | ||
| + "from contentlet where inode = ? and mod_date <= ? for update") |
There was a problem hiding this comment.
🟡 Medium severity blocking issue identified in your code:
The method identified is susceptible to injection. The input should be validated and properly
escaped.
Why this might be safe to ignore:
The SQL is assembled from fixed query fragments, and the inode and date values are bound through placeholders with addParam rather than interpolated into the statement. No attacker-controlled input reaches SQL syntax in this code.
To resolve this comment:
🔧 No guidance has been designated for this issue. Fix according to your organization's approved methods.
💬 Ignore this finding
Reply with Semgrep commands to ignore this finding.
/fp <comment>for false positive/ar <comment>for acceptable risk/other <comment>for all other reasons
Alternatively, triage in Semgrep AppSec Platform to ignore the finding created by CUSTOM_INJECTION-2.
If this is a critical or high severity finding, please also link this issue in the #security channel in Slack.
You can view more details about this finding in the Semgrep AppSec Platform.
| final var candidates = new DotConnect().setSQL("select inode from contentlet where structure_inode = ? " | ||
| + "and inode > ? and mod_date <= ? order by inode limit 100") |
There was a problem hiding this comment.
🟡 Medium severity blocking issue identified in your code:
The method identified is susceptible to injection. The input should be validated and properly
escaped.
Why this might be safe to ignore:
The SQL is built only from fixed query text, while type, cursor, and date values are supplied separately through placeholders and addParam. No attacker-controlled input is interpolated into the SQL syntax.
To resolve this comment:
🔧 No guidance has been designated for this issue. Fix according to your organization's approved methods.
💬 Ignore this finding
Reply with Semgrep commands to ignore this finding.
/fp <comment>for false positive/ar <comment>for acceptable risk/other <comment>for all other reasons
Alternatively, triage in Semgrep AppSec Platform to ignore the finding created by CUSTOM_INJECTION-2.
If this is a critical or high severity finding, please also link this issue in the #security channel in Slack.
You can view more details about this finding in the Semgrep AppSec Platform.
|
Semgrep found 1 🟡 Medium severity issue identified in your code: The method identified is susceptible to injection. The input should be validated and properly If this is a critical or high severity finding, please also link this issue in the #security channel in Slack. |
|
Pull Request Unsafe to Rollback!!!
|
|
Pull Request Unsafe to Rollback!!!
Pull Request Unsafe to Rollback!!!
Both findings are conditional on |
badcb59 to
959df10
Compare
165a660 to
5824a30
Compare
|
test connectivity check - ignore |
|
Pull Request Unsafe to Rollback!!!
Note: both risks are conditional on |
5824a30 to
ea2656b
Compare
959df10 to
666a133
Compare
ea2656b to
f874dda
Compare
666a133 to
7e8397b
Compare
|
Pull Request Unsafe to Rollback!!!
|
nollymar
added
the
PR : dotbot review
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| final String backup = ContentletBackupStorage.getInstance().storeField( | ||
| row.get("identifier").toString(), inode, json, binaries, List.copyOf(metadata)); | ||
| ((ObjectNode) document.path("fields")).remove(field); | ||
| new DotConnect().setSQL("update contentlet set contentlet_as_json = ?::jsonb where inode = ?") |
There was a problem hiding this comment.
🔴 [P1] BinaryFieldCleanupProcessor.java:183 make JSON update portable
Current code:
new DotConnect().setSQL("update contentlet set contentlet_as_json = ?::jsonb where inode = ?")
.addParam(JSON.writeValueAsString(document)).addParam(inode).loadResult();Problem: Hardcoded ::jsonb cast fails on MySQL, MSSQL and Oracle.
Fix:
new DotConnect().setSQL("update contentlet set contentlet_as_json = "
+ (DbConnectionFactory.isPostgres() ? "?::jsonb" : "?") + " where inode = ?")
.addParam(JSON.writeValueAsString(document)).addParam(inode).loadResult();
|
|
||
| private final Lazy<ObjectMapper> defaultMapper = Lazy.of(() -> { | ||
| final ObjectMapper objectMapper = new ObjectMapper(); | ||
| if (com.dotcms.storage.AssetStorageFeature.isEnabled()) { |
There was a problem hiding this comment.
🟡 [P2] JacksonMarshalUtilsImpl.java:27 flag-on ParameterNamesModule alters the shared global mapper
Current code:
if (com.dotcms.storage.AssetStorageFeature.isEnabled()) {
objectMapper.registerModule(new com.fasterxml.jackson.module.paramnames.ParameterNamesModule(
com.fasterxml.jackson.annotation.JsonCreator.Mode.PROPERTIES));
}Problem: Flag-on registration changes constructor selection for every MarshalUtils caller (push-publish bundles, system events, workflow payloads), not just binary types. Mapper behavior also depends on the flag's value at first lazy use.
Fix:
// Scope ParameterNamesModule to a dedicated mapper used only for the binary field
// immutables, leaving the shared defaultMapper configuration unchanged.
…3 asset storage Third slice of the S3 asset storage work. With FEATURE_FLAG_S3_ASSET_STORAGE on, content check-in stores immutable binary revisions and records their storage keys in the content JSON, metadata generation restores evicted originals and shares byte-derived extraction, and deletion uses durable binaryAssetCleanup and binaryFieldCleanup jobs plus verified S3 recovery archives. The S3 cleanup processors do not register while the flag is off, and flag-off behavior matches main.
Field-removal archiving used to run as one transaction over every version of a content type, holding FOR UPDATE locks on each batch while it uploaded recovery ZIPs to S3, so large types blocked edits and risked timeouts, and one failure rolled back and repeated all the work. Each row is now archived in its own short transaction that locks only that row, rechecks the deletion timestamp, and commits a compare-and-set afterInode cursor with it, so a retry resumes after the last archived row. ContentletAPI.cleanField queues the same job instead of uploading inline. Flag-off behavior is unchanged.
…ache lease only until open
… store from the storage provider
…and per-language binaries Whole-inode cleanup now records the inode's exact binary and metadata paths when the deletion enqueues the job, and the worker deletes only those paths. An inode re-created after deletion (push publishing keeps the sender's inodes) keeps the revisions it uploads. The "content version still exists" refusal is unchanged, and a job without a recorded inventory deletes nothing. FileMetadataAPI.removeMetadataForInode is split into listMetadataForInode and removeMetadataPaths for this. BinaryAssetCleanupProcessor now implements Validator and rejects submissions through the public job endpoint, as BinaryFieldCleanupProcessor already does. A check-in that uploads a new revision registers a rollback listener that deletes that revision and its revision metadata. The key carries a fresh UUID, so no other version can reference it. A failed delete is logged and never thrown. With the flag on, deleting one language of multilingual content now records recovery archives and cleanup jobs like the other deletion paths, and ContentletAPI.delete(Contentlet) runs in a transaction so those jobs commit with the deleted rows (Site Browser and WebDAV call it without one). The flag-off paths are unchanged. deleteBinaryFiles enqueues one cleanup job per distinct inode, since callers pass duplicated version lists. BinaryExporterServlet releases the cache lease once the file to stream is open, so a slow client no longer defers eviction. DropOldContentletRunner.deleteFromAssetsDir is restored to main; its flag-on branch was unreachable. The S3 storage doc is updated to match, and the unit and flag-on integration tests cover the exact inventory, the endpoint rejection, the rollback deletion, the per-language path and the single job per inode. The S3 metadata outage test now expects an unreadable local copy to be replaced from S3 when S3 holds a readable one, and to fail without deleting it when it does not, matching the storage layer. The S3 servlet response test now expects eviction to proceed once the full response has opened its file, and to be refused in the range branch until that branch opens it.
…r its key names Contentlet.getBinary resolved a revision key against the contentlet's current inode. A contentlet that carries the previous version's revision under a new inode (as check-in does after it assigns the new inode) failed with "File System error." once that revision was evicted from the local cache, because the key belongs to the old inode. getBinary now restores the revision under the owner its key names; BinaryAssetReference.localFile still validates the key. Check-in does not hit this today only because saving the content JSON restores the file while hydrating metadata, before check-in reads the binary. Refs #37868
3523198 to
236a0d2
Compare
7e8397b to
14d49ce
Compare
| final String backup = ContentletBackupStorage.getInstance().storeField( | ||
| row.get("identifier").toString(), inode, json, binaries, List.copyOf(metadata)); | ||
| ((ObjectNode) document.path("fields")).remove(field); | ||
| new DotConnect().setSQL("update contentlet set contentlet_as_json = ?::jsonb where inode = ?") |
There was a problem hiding this comment.
🔴 [P1] BinaryFieldCleanupProcessor.java:183 make JSON update portable
Current code:
new DotConnect().setSQL("update contentlet set contentlet_as_json = ?::jsonb where inode = ?")Problem: Hardcoded ::jsonb cast fails on MySQL, MSSQL and Oracle.
Fix:
new DotConnect().setSQL("update contentlet set contentlet_as_json = "
+ (DbConnectionFactory.isPostgres() ? "?::jsonb" : "?") + " where inode = ?")
|
dotbot code review:
Flag-on field cleanup hardcodes Postgres-only ::jsonb for contentlet_as_json while the same patch branches on isPostgres() elsewhere, breaking cleanup on other supported databases. Tip: comment with "/dotbot address comments" to attempt automated fixes for unresolved review threads. reviewed by dotbot · meta/muse-spark-1.3 · medium |
| for (String path : storage.listObjectPaths(metadataGroup(), parent)) { | ||
| final String absolute = path.startsWith("/") ? path : "/" + path; | ||
| if (parent.equals(legacyParent) && absolute.substring(parent.length()).contains("/")) continue; | ||
| if (ownsMetadata(owner, absolute)) metadata.add(absolute); |
There was a problem hiding this comment.
🟡 [P2] BinaryFieldCleanupProcessor.java:183 cursor update uses Postgres-only JSONB concatenation
Current code:
final var saved = new DotConnect().setSQL("update job set parameters = parameters || ?::jsonb, "
+ "updated_at = current_timestamp where id = ? and queue_name = ? "
+ "and coalesce(parameters->>'afterInode', '') = ? returning id")Problem: || ?::jsonb concatenation, parameters->>'...' extraction and returning id are Postgres-only; this job fails at runtime on MySQL/MSSQL/Oracle even before the archived row update.
Fix:
final var saved = new DotConnect().setSQL("update job set parameters = "
+ (DbConnectionFactory.isPostgres() ? "parameters || ?::jsonb" : "?")
+ ", updated_at = current_timestamp where id = ? and queue_name = ? "
+ (DbConnectionFactory.isPostgres() ? "and coalesce(parameters->>'afterInode', '') = ? returning id" : "...")The cursor compare/merge needs per-DB SQL (e.g., MySQL JSON_MERGE_PATCH(parameters, ?) and JSON_UNQUOTE(JSON_EXTRACT(parameters, '$.afterInode')), plus a follow-up select for rowcount). If only Postgres is supported for the S3 lifecycle, gate the flag on isPostgres().
| contentletRaw); | ||
|
|
||
| if (com.dotcms.storage.AssetStorageFeature.isEnabled() && !contentType.fields(BinaryField.class).isEmpty()) { | ||
| // Persist immutable binary references alongside the filename in this transaction. |
There was a problem hiding this comment.
🟡 [P2] ESContentletAPIImpl.java:6181 unconditionally writes contentlet_as_json, overriding SAVE_CONTENTLET_AS_JSON=false
Current code:
if (com.dotcms.storage.AssetStorageFeature.isEnabled() && !contentType.fields(BinaryField.class).isEmpty()) {
final String json = APILocator.getContentletJsonAPI().toJson(contentlet);
final String jsonValue = DbConnectionFactory.isPostgres() ? "?::jsonb" : "?";
new DotConnect().setSQL("update contentlet set contentlet_as_json = " + jsonValue + " where inode = ?")Problem: With the flag on, every binary-content checkin writes contentlet_as_json even when the system property SAVE_CONTENTLET_AS_JSON is disabled, silently repopulating a column operators deliberately keep null.
Fix:
if (com.dotcms.storage.AssetStorageFeature.isEnabled()
&& Config.getBooleanProperty("SAVE_CONTENTLET_AS_JSON", true)
&& !contentType.fields(BinaryField.class).isEmpty()) {If the S3 lifecycle genuinely requires the JSON, document that enabling FEATURE_FLAG_S3_ASSET_STORAGE implies JSON persistence, or fail fast when SAVE_CONTENTLET_AS_JSON=false instead of silently overriding it.
| final var rows = new DotConnect().setSQL("select contentlet_as_json from contentlet where inode = ? for update") | ||
| .addParam(requested.getInode()).loadObjectResults(); | ||
| if (rows.isEmpty()) throw new DotDataException("Content version disappeared before backup"); | ||
| final Object persisted = rows.getFirst().get("contentlet_as_json"); |
There was a problem hiding this comment.
🟡 [P2] ContentletBackupStorage.java:73 identifier of restored contentlet used before existence check
Current code:
final Contentlet content = persisted == null || persisted.toString().isBlank()
? new Contentlet(APILocator.getContentletAPI().find(requested.getInode(), APILocator.systemUser(), false))
: APILocator.getContentletJsonAPI().toMutableContentlet(
ContentletJsonHelper.INSTANCE.get().immutableFromJson(persisted.toString()));
validateId(content.getIdentifier());Problem: When contentlet_as_json is blank, ContentletAPI.find returns null or throws after the row was already locked for update; validateId(content.getIdentifier()) on null content NPEs inside a transaction, aborting the deletion.
Fix:
final Contentlet content = persisted == null || persisted.toString().isBlank()
? APILocator.getContentletAPI().find(requested.getInode(), APILocator.systemUser(), false)
: APILocator.getContentletJsonAPI().toMutableContentlet(
ContentletJsonHelper.INSTANCE.get().immutableFromJson(persisted.toString()));
if (content == null) throw new DotDataException("Content version disappeared before backup");
validateId(content.getIdentifier());| // S3 cleanup jobs must commit with the deleted rows, and callers such as the Site | ||
| // Browser and WebDAV reach this method without a transaction. | ||
| final boolean[] result = new boolean[1]; | ||
| LocalTransaction.wrap(() -> result[0] = this.deleteContentlets(contentlets, user, |
There was a problem hiding this comment.
🟡 [P2] ESContentletAPIImpl.java:2757 commit listener registered after LocalTransaction already committed
Current code:
final boolean[] result = new boolean[1];
LocalTransaction.wrap(() -> result[0] = this.deleteContentlets(contentlets, user,
respectFrontendRoles, isSite));
deleted = result[0];Problem: For non-transactional callers (Site Browser/WebDAV), LocalTransaction.wrap commits inside; the ContentletDeletedEvent commit listener is added after commit with no active transaction and never fires.
Fix:
LocalTransaction.wrap(() -> {
result[0] = this.deleteContentlets(contentlets, user, respectFrontendRoles, isSite);
HibernateUtil.addCommitListener(() -> this.localSystemEventsAPI.notify(
new ContentletDeletedEvent<>(contentlet, user)));
});
deleted = result[0];Assumption: HibernateUtil.addCommitListener no-ops without a current transaction. What to verify: subscribers of ContentletDeletedEvent still run for non-transactional deletes with the flag on.
|
dotbot code review:
4 new actionable findings were identified in the current changes, and 1 prior unresolved dotbot finding still applies, so the patch remains incorrect. All behavioral changes are gated behind FEATURE_FLAG_S3_ASSET_STORAGE (default off, verified across call sites) and covered by extensive new tests. The found issues (Postgres-only SQL fragments, JSON-column override, backup NPE edge, post-commit listener) are all flag-on and addressable without blocking the stacked rollout, and the flag-on rollback limitation is explicitly documented as intentional. Tip: comment with "/dotbot address comments" to attempt automated fixes for unresolved review threads. reviewed by dotbot · ~z-ai/glm-latest · medium |
|
Pull Request Unsafe to Rollback!!!
Both findings are conditional on operators setting |
This branch has not been deployed
3 participants
Refs #37868
Proposed Changes
This is where content starts using S3, and the PR with the most changes to heavily used classes (
ESContentletAPIImpl,Contentlet,FileMetadataAPIImpl, the JSON and XML serializers). Every change is gated..../{field}/.revisions/{revision-id}/{filename}and the Binary field JSON gainsstorageKeyandmetadataStorageKey. The reference changes inside the content transaction, so a rollback keeps the previous revision, and the revision a rolled-back check-in uploaded is deleted. Legacy JSON and keys stay readable.SharedExtractedMetadata), metadata keys follow the binary revision, and custom metadata is copied to replacements.binaryAssetCleanupjob per inode in the deleting transaction. Each job records the exact paths stored at that moment and deletes only those, so a revision uploaded later under a reused inode (as a push-publishing receiver does) is never touched. Binary field deletion recordsbinaryFieldCleanupjobs that archive, verify and then delete. Both use the existing job queue and its retry policy, and both reject direct submissions through the public job endpoint.BACKUP_DELETED_CONTENTLETS_TO_DISKalso on, deletion first writes a verified recovery ZIP to thedeleted-content-backupsgroup; a failed backup aborts the deletion.AssetStorageFeature.allowsJobProcessorkeeps the S3 job processors out ofJobQueueManagerAPIImplwhile the flag is off.BinaryExporterServlet),FileAsset.getInputStreamandContentlet.getBinaryStreamhold a cache lease only until the file is open, so a slow client cannot defer eviction. That is safe on a local disk because an open file survives eviction; eviction on an NFS asset directory has not been validated.Behavior with the flag off
Unchanged from main: no revision keys are written, no S3 cleanup processors are registered, and the
deleteAllVersionsandBackupinterceptor stays the existing no-op. The flag-off integration run below exists to prove this for the classes every check-in goes through.Review fixes
The commit
fix(storage): make whole-inode cleanup exact and reclaim rolled-back and per-language binariesaddresses a full review of this PR. All of it is flag-on only:binaryAssetCleanupvalidates submissions the same waybinaryFieldCleanupdoes.ContentletAPI.delete(Contentlet, User, boolean)runs in a transaction with the flag on, because the Site Browser and WebDAV call it without one. This one goes beyond the review findings, so it is worth a look.BinaryExporterServletreleases its lease once the file is open, as described above.DropOldContentletRunner.deleteFromAssetsDiris removed.Second review fix
A second review found that
Contentlet.getBinarylooked up a stored revision under the contentlet's current inode. A contentlet that carries the previous version's binary under a new inode, which is what check-in has after it assigns the new inode, threwFile System error.once that revision had been evicted from the local cache, because the revision key names the old inode. The last commit (fix(storage): restore a carried-forward binary revision from the owner its key names) restores the revision under the owner its key names.BinaryAssetReference.localFilestill validates the key. Flag-on only.Check-in does not hit this today: saving the content JSON hydrates metadata, which restores the evicted file before check-in reads the binary. The fix keeps
getBinarycorrect on its own instead of relying on that order.Rollback safety (please read)
Enabling the flag from this PR on is one-way for content written while it is on: check-in stores the binary only under a revision key that neither an older release nor this release with the flag off reads. Leaving the flag off, the default, is safe to roll back. The doc's "Enabling the flag is not rollback-safe" section says the same. This PR should carry the rollback-unsafe label.
Deliberately not in this PR
Temporary-upload metadata (5), backfill and starter import/export (3), publishing (4), renditions (6).
BinaryCleanupJobchanges entirely in 5.Checklist
ContentletAPITest(183),FileMetadataAPITest(48),MetadataDelegateTestand the three new classes; one index-count case (testRemoveContentFromIndexMultilingualContent) failed once and passed on rerun, in two separate flag-off runs. Flag on against MinIO: 17 run, 0 failures, 0 skips. After the review fixes, the integration suites were run on the top of the stack (feat(storage): serve renditions, compiled CSS and templates through S3 asset storage #37776), flag off and on. The three new integration classes are registered inJunit5Suite1; their S3 cases skip in CI, which runs flag-off. After the second fix, flag on against MinIO:BinaryAssetStorageIntegrationTest10 run, 0 failures, and its new case fails without the fix;BinaryAssetReferenceTest7 run, 0 failures.